Using SystemTime::now() for jitter calculation can lead to zero jitter on systems with low clock resolution (e.g., 1ms resolution in VMs, containers, or Windows). Since base_delay.as_millis() is at most 50, and 1,000,000 (1ms in nanoseconds) is divisible by all common backoff ceilings (2, 4, 8, 16, 32, 50), the modulo operation elapsed.as_nanos() % jitter_ceiling will always evaluate to exactly 0. This completely defeats the purpose of jitter under high contention.

Instead, use std::collections::hash_map::RandomState to obtain a high-quality, thread-safe, and un-correlated pseudo-random value seeded from the OS without adding external dependencies.

Fixed in 0eaf69d — jitter now derives from a RandomState-seeded hash (clock-independent), so it no longer collapses to 0 on coarse-resolution clocks. See cas_retry_backoff in crates/ironclaw_filesystem/src/cas.rs.

According to the repository's general rules, async helper functions that return futures intended to be Send should explicitly add Send bounds to their closure generic constraints. This ensures that any non-Send regressions are caught at the helper's definition site rather than at the call site.

pub async fn cas_update<F, S, T, E, D, N, A, Fut>(
    filesystem: &ScopedFilesystem<F>,
    scope: &ResourceScope,
    path: &ScopedPath,
    decode: D,
    encode: N,
    mut apply: A,
) -> Result<T, CasUpdateError<E>>
where
    F: RootFilesystem + ?Sized,
    S: PartialEq + Clone + Send,
    T: Send,
    E: Send,
    D: Fn(&[u8]) -> Result<S, E> + Send,
    N: Fn(&S) -> Result<Entry, E> + Send,
    A: FnMut(Option<S>) -> Fut + Send,
    Fut: Future<Output = Result<CasApply<S, T>, E>> + Send,

Considered but declined. Adding Send to the helper generics forces propagating Send across consumer-crate generics (run_state mutate: M, turns persistence T) — threading a bound through multiple layers for marginal "catch at the def site" benefit, which .claude/rules/architecture.md flags as bound-creep. The futures are already Send-constrained where awaited/spawned.

According to the repository's general rules, async helper functions that return futures intended to be Send should explicitly add Send bounds to their closure generic constraints. This ensures that any non-Send regressions are caught at the helper's definition site rather than at the call site.

async fn cas_update_loop<F, S, T, E, D, N, A, Fut>(
    filesystem: &ScopedFilesystem<F>,
    scope: &ResourceScope,
    path: &ScopedPath,
    decode: &D,
    encode: &N,
    apply: &mut A,
) -> Result<T, CasUpdateError<E>>
where
    F: RootFilesystem + ?Sized,
    S: PartialEq + Clone + Send,
    T: Send,
    E: Send,
    D: Fn(&[u8]) -> Result<S, E> + Send,
    N: Fn(&S) -> Result<Entry, E> + Send,
    A: FnMut(Option<S>) -> Fut + Send,
    Fut: Future<Output = Result<CasApply<S, T>, E>> + Send,

Same decision as the cas_update bound above — declined to avoid propagating Send across consumer-crate generics for marginal benefit.

Medium — Discarded approvals now keep the full request payload on disk.

discard_pending now rewrites the filesystem record as Discarded instead of deleting it, but it keeps the full original ApprovalRequest body in the tombstone. That means a cancelled approval's action, reason, requester, and fingerprint remain recoverable from disk/backups even though public APIs hide discarded records. This is a data-minimization regression on an approval control-plane record.

Fix: Store only the minimal tombstone data needed to reserve the request ID, or keep the discarded-ID reservation in a compact side index instead of retaining the full approval payload.

Deferred (consistency, not a discard-specific bug). ApprovalRecord.request is non-optional and Approved/Denied records already retain the identical full payload on disk — the Discarded tombstone is consistent with every other terminal state. Singling out discard for minimization would be the inconsistency. If approval-payload retention matters it's a cross-cutting policy (pruning/redaction of all terminal approval records) plus a schema change (optional/redacted request), which is out of scope for this PR. Tracking as a retention follow-up.

Medium — Missing test for op-time unsupported-write fail-closed path.

The helper covers the pre-flight non-CAS rejection, but the second fail-closed path is untested: an unknown/default-capability backend can read successfully and then return FilesystemError::Unsupported { operation: WriteFile } from put. That branch maps to CasUpdateError::CasUnsupported and should be protected because it is the composite-router fallback path.

Fix: Add tests::cas::unsupported_write_file_maps_to_cas_unsupported using a default-capabilities backend whose read succeeds and whose CAS write returns Unsupported(WriteFile).

Fixed in 578e971 — added unsupported_write_file_maps_to_cas_unsupported: a default/unknown-capability backend whose get succeeds and whose put returns Unsupported{WriteFile}, asserting cas_update maps it to CasUpdateError::CasUnsupported. This exercises the op-time fallback path, distinct from the already-tested pre-flight rejection.

Medium — CAS helper clones the full snapshot on every retry.

cas_update clones the decoded snapshot before every apply, and several migrated call sites clone again when building CasApply::new(...). For large snapshots such as turn state, each write now copies the whole record at least twice, and contention multiplies that cost across the retry loop. That is avoidable churn on the hot persistence path this PR is trying to improve.

Fix: Pass apply a borrowed snapshot or split the helper API so the helper retains one owned copy for equality/write checks while call sites clone only when they truly need ownership.

Deferred (low-value perf). Confirmed two in-memory clones per attempt: cas_update clones current (needed — retained for the equality check after apply consumes its copy) and the turns call site clones new_snapshot (threaded into outcome to populate the read cache). Both are struct copies, not IO, and the loop only re-clones on VersionMismatch (rare). Eliminating them requires an API change (return the final snapshot from cas_update, or hash-based equality) for marginal gain — premature without a profile showing turn-state writes are hot. Will revisit with a benchmark.

Medium — Turn-state apply has become a 1k-line orchestration knot.

This refactor pushes FilesystemTurnStateStore from 961 to 1051 lines, and the new apply flow now mixes cache invalidation, runner-lease overlay, transient store construction, FnMut adaptation, CAS no-op detection, timeout policy, and cache update/rollback in one method. That is a lot of state to hold in one control flow, especially in the turn-state persistence path.

Fix: Split the CAS-specific orchestration into smaller helpers, or move runner-lease overlay/cache bookkeeping behind a focused helper so apply coordinates the transition at a higher level.

Agreed it's doing a lot, deferred to #5274. The clean move is extracting the per-retry transform (overlay + store build + no-op detection) into a named helper so apply reads as orchestration. Follow-up #5274 (migrate the runner-lease sidecar onto cas_update) already rewrites this exact apply/CAS path, so the decomposition lands there — once — rather than re-churning a just-merged file twice. Added to #5274's scope. (Note: most of the 961→1051 growth is main's own module split landing via the merge; net the file is smaller than this branch's pre-merge 1455.)

Medium — The apply timeout is now defined in two places.

The turn store keeps its own 15-second FILESYSTEM_APPLY_TIMEOUT, while the shared CAS helper now owns the same 15-second loop timeout. The outer timeout comment says this may be shorter for tests, but the default value is still duplicated, so a future timeout-policy change can drift the turn store away from the helper contract.

Fix: Use the shared ironclaw_filesystem::FILESYSTEM_APPLY_TIMEOUT as the turn-store default, or remove the outer default when the helper deadline is the only intended policy.

Fixed in 578e971 — the turn store now uses the shared ironclaw_filesystem::FILESYSTEM_APPLY_TIMEOUT instead of a duplicated local constant; the per-store override via with_apply_timeout (used in tests) is unchanged.

Medium — cas_update lacks a generic backend-error regression test.

The helper has coverage for first write, no-op, contention, timeout, and unsupported-write, but not the fallback Err(error) => CasUpdateError::Backend(error) branch on a normal write failure. A backend returning a non-Unsupported FilesystemError from put would still be unpinned here.

Fix: Add tests::cas::backend_put_error_maps_to_backend covering a successful read followed by a generic backend error from put.

Fixed in 4b061dd — added backend_put_error_maps_to_backend: a CAS-capable backend whose get succeeds and whose put returns a generic (non-VersionMismatch, non-Unsupported{WriteFile}) FilesystemError, asserting cas_update maps it to CasUpdateError::Backend(_). Covers the catch-all arm at cas.rs:341.

Low — Last retry still pays a backoff sleep.

The loop always awaits cas_retry_backoff(attempt) on VersionMismatch, including the final permitted attempt. That adds an avoidable 2-50ms of tail latency to every exhausted CAS call on the shared hot path, with no chance of success because no retry remains.

Fix: Skip the sleep when attempt + 1 == FILESYSTEM_CAS_RETRIES and return RetriesExhausted immediately.

Fixed in 4b061dd — the VersionMismatch arm now guards the sleep with if attempt + 1 < FILESYSTEM_CAS_RETRIES, so the final exhausted attempt returns RetriesExhausted immediately without the 2-50ms backoff. Retry count unchanged.

Low — Discarded approval status is untested.

The new ApprovalStatus::Discarded arm has no direct regression test. Nothing asserts that discarded approvals map to ApprovalDiscarded, so this one-line branch could drift silently.

Fix: Add tests::helpers::approval_not_approved_error_kind_maps_discarded_status covering ApprovalStatus::Discarded -> "ApprovalDiscarded".

Fixed in 4b061dd — added approval_not_approved_error_kind_maps_discarded_status asserting ApprovalStatus::Discarded -> "ApprovalDiscarded".

Low — Fix the equality-path example in the public API docs.

The public CasApply docs describe the unchanged-snapshot fast path as Some(existing) == snapshot, but snapshot is an S while current is the Option<S>. As written, the example is type-mismatched and makes the two no-op modes harder to distinguish for store authors reading the API surface.

Fix: Rewrite the sentence in terms of the actual comparison, for example current is Some(existing) and the returned snapshot equals existing.

Fixed in 4b061dd — the CasApply doc example no longer mismatches types; it now describes the actual check (current is Some(existing) and the returned snapshot equals existing), matching matches!(&current, Some(e) if *e == snapshot) and distinguishing it from the explicit CasApply::no_op path.

High — Byte-only backends still slip through on first write.

The fail-closed gate only rejects backends that advertise a non-default capability shape or that return Unsupported(WriteFile) from put. A composite filesystem reports BackendCapabilities::default() before path resolution, then routes the write to the mounted backend. LocalFilesystem accepts CasExpectation::Absent for plain byte entries, and CAS snapshots are JSON byte entries, so the first snapshot write on a local/byte-only mount can succeed instead of surfacing CasUnsupported. Later Version(...) writes fail, but the helper has already violated its no-CAS fail-closed contract for fresh records.

Fix: Do not infer CAS support from the root composite capabilities alone. Gate against path-resolved mount capabilities, or reject unknown/default roots before issuing any write so Absent writes cannot succeed on non-CAS mounts.

Fixed in 0d4bc1c — made all CAS-store encoders record-shaped (entry.kind = Some(RecordKind)), so LocalFilesystem's existing record-shaped rejection fires fail-closed uniformly on the Absent first-write too (turns was already protected; run_state/secrets/resources/threads were not). Self-enforcing at the data-model level, no new helper branches. Severity is really Medium — Absent is create-if-absent and all updates fail-closed, so there is no blind overwrite / lost update; only creation succeeded then updates failed loudly. New record_shaped_first_write_fails_closed_on_byte_only_backend + a store-level fail-closed test over a real byte-only LocalFilesystem mount; verified across the postgres/libsql/all-features matrix.

Medium — Missing test for backend read failure propagation.

The filesystem.get(...).await error arm now returns CasUpdateError::Backend, but the new CAS helper tests never inject a read failure. The suite covers contention, capability gating, timeout, and put failures, but not the branch that should surface a backend failure before a snapshot can be loaded.

Fix: Add tests::cas::get_error_is_returned_as_backend using a backend whose get returns a non-NotFound FilesystemError and assert cas_update returns CasUpdateError::Backend(_).

Fixed in 0d4bc1c — added backend_get_error_maps_to_backend pinning a get failure to CasUpdateError::Backend.